fix(cli): respect ignore files in adk deploy commands #4187
Conversation
The adk deploy commands (cloud_run, agent_engine, gke) were not properly respecting .gitignore, .gcloudignore, or .ae_ignore files, causing unwanted files (like venv, .git, etc.) to be uploaded. This change: - Adds a unified _get_ignore_patterns_func helper that reads all three ignore files. - Updates to_cloud_run, to_agent_engine, and to_gke to use this helper. - Removes hardcoded ignore patterns to strictly follow user configuration. - Adds comprehensive unit tests to verify the fix. Fixes google#4183
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a great improvement by making the adk deploy commands respect various ignore files (.gitignore, .gcloudignore, .ae_ignore). The implementation is clean, centralizing the logic in a new _get_ignore_patterns_func helper. This function is then correctly applied to to_cloud_run, to_agent_engine, and to_gke deployment functions. The refactoring in to_agent_engine is particularly nice as it replaces specific logic with the new generic helper. The addition of comprehensive unit tests in test_cli_deploy_ignore.py ensures the new functionality is well-tested and robust.
I have one suggestion to make the error handling in the new helper function more specific and robust. Overall, this is a solid contribution.
| try: | ||
| with open(filepath, 'r') as f: | ||
| for line in f: | ||
| line = line.strip() | ||
| if line and not line.startswith('#'): | ||
| # If it ends with /, remove it for fnmatch compatibility | ||
| if line.endswith('/'): | ||
| line = line[:-1] | ||
| patterns.add(line) | ||
| except Exception as e: | ||
| click.secho(f'Warning: Failed to read {filename}: {e}', fg='yellow') |
There was a problem hiding this comment.
This try...except block can be made more robust.
- Specify file encoding: It's good practice to explicitly specify the file encoding when opening text files to ensure consistent behavior across different systems.
utf-8is a safe default for ignore files. - Use specific exceptions: Catching the generic
Exceptionis too broad and can hide unexpected bugs. It's better to catch more specific exceptions, such asOSErrorfor file I/O problems andUnicodeDecodeErrorwhen an encoding is specified.
| try: | |
| with open(filepath, 'r') as f: | |
| for line in f: | |
| line = line.strip() | |
| if line and not line.startswith('#'): | |
| # If it ends with /, remove it for fnmatch compatibility | |
| if line.endswith('/'): | |
| line = line[:-1] | |
| patterns.add(line) | |
| except Exception as e: | |
| click.secho(f'Warning: Failed to read {filename}: {e}', fg='yellow') | |
| try: | |
| with open(filepath, 'r', encoding='utf-8') as f: | |
| for line in f: | |
| line = line.strip() | |
| if line and not line.startswith('#'): | |
| # If it ends with /, remove it for fnmatch compatibility | |
| if line.endswith('/'): | |
| line = line[:-1] | |
| patterns.add(line) | |
| except (OSError, UnicodeDecodeError) as e: | |
| click.secho(f'Warning: Failed to read {filename}: {e}', fg='yellow') |
3 participants
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Problem:
The
adk deploycommands (cloud_run,agent_engine,gke) were not properly respecting.gitignore,.gcloudignore, or.ae_ignorefiles. This caused all files in the source directory, including large or sensitive ones likevenv/,.git/, and.env, to be copied to the temporary staging directory and subsequently uploaded to hosted environments.Solution:
_get_ignore_patterns_funchelper insrc/google/adk/cli/cli_deploy.pythat reads and combines patterns from.gitignore,.gcloudignore, and.ae_ignore.to_cloud_run,to_agent_engine, andto_gketo use this helper as an ignore filter inshutil.copytree.Testing Plan
Unit Tests:
Summary of
pytestresults:Manual End-to-End (E2E) Tests:
Verified the fix by following these steps:
.gitignorefile.shutil.rmtreecleanup in code to inspect./debug_staged_out.agent.pyand.gitignorewere present, butignored_file.txtwas correctly excluded from the staging area.Checklist